Skip to content

Core: Use exsolve resolveModulePath for safeResolveModule#32477

Merged
ndelangen merged 11 commits into
storybookjs:nextfrom
mrginglymus:more-win-fix
Sep 25, 2025
Merged

Core: Use exsolve resolveModulePath for safeResolveModule#32477
ndelangen merged 11 commits into
storybookjs:nextfrom
mrginglymus:more-win-fix

Conversation

@mrginglymus

@mrginglymus mrginglymus commented Sep 16, 2025

Copy link
Copy Markdown
Contributor

What I did

Use exsolve for safeResolveModule, as this can handle absolute paths on windows (which fileURLToPath(import.meta.resolve(...)) cannot).

This does mean we bypass the importMetaResolve check for vitest...I'm hoping that for this specific use case its ok.

The linked issue in that code (vitest-dev/vitest#6953) is now closed for vite 7

I've also updated the yarn pnp sandbox to wrap frameworks and addons in getAbsolutePath.

Checklist for Contributors

Testing

The changes in this PR are covered in the following automated tests:

  • stories
  • unit tests
  • integration tests
  • end-to-end tests

Manual testing

Just running a sandbox on windows should be enough to demonstrate this.

Documentation

  • Add or update documentation reflecting your changes
  • If you are deprecating/removing a feature, make sure to update
    MIGRATION.MD

Checklist for Maintainers

  • When this PR is ready for testing, make sure to add ci:normal, ci:merged or ci:daily GH label to it to run a specific set of sandboxes. The particular set of sandboxes can be found in code/lib/cli-storybook/src/sandbox-templates.ts

  • Make sure this PR contains one of the labels below:

    Available labels
    • bug: Internal changes that fixes incorrect behavior.
    • maintenance: User-facing maintenance tasks.
    • dependencies: Upgrading (sometimes downgrading) dependencies.
    • build: Internal-facing build tooling & test updates. Will not show up in release changelog.
    • cleanup: Minor cleanup style change. Will not show up in release changelog.
    • documentation: Documentation only changes. Will not show up in release changelog.
    • feature request: Introducing a new feature.
    • BREAKING CHANGE: Changes that break compatibility in some way with current major version.
    • other: Changes that don't fit in the above categories.

🦋 Canary release

This PR does not have a canary release associated. You can request a canary release of this pull request by mentioning the @storybookjs/core team here.

core team members can create a canary release here or locally with gh workflow run --repo storybookjs/storybook canary-release-pr.yml --field pr=<PR_NUMBER>

Greptile Summary

Updated On: 2025-09-16 12:22:17 UTC

This PR addresses a Windows compatibility issue in Storybook 10.0.0-beta.3 by replacing the module resolution implementation in the safeResolveModule function. The change switches from using Node.js's native import.meta.resolve() combined with fileURLToPath() to the exsolve library's resolveModulePath function.

The core problem was that the previous implementation would throw ERR_INVALID_URL_SCHEME errors when handling absolute paths on Windows systems. The exsolve library provides better cross-platform compatibility and can properly resolve absolute paths on Windows where the native Node.js resolution methods fail.

The implementation maintains the same API and functionality while simplifying the code structure. Instead of manually looping through file extensions and using multiple try-catch blocks, the new approach uses exsolve's built-in extension handling and only re-throws errors that are not ERR_MODULE_NOT_FOUND. This change is part of Storybook's module resolution utilities that are used throughout the codebase for finding and loading various components and configuration files.

Confidence score: 4/5

  • This is a targeted bug fix that addresses a specific Windows compatibility issue with a well-tested third-party library
  • The change maintains the same API contract and error handling patterns as the previous implementation
  • Focus attention on the code/core/src/shared/utils/module.ts file to ensure the exsolve integration works correctly across different environments

Summary by CodeRabbit

  • Bug Fixes

    • Improved reliability of module resolution across ESM, PNPM, and Yarn PnP environments.
    • Angular preset now resolves paths as file URLs to prevent resolution errors.
    • Stabilized CLI automigrations and telemetry path lookups.
  • Refactor

    • Simplified path construction by replacing join-based logic with template literals.
  • Documentation

    • Updated migration guide and configuration snippets to the new getAbsolutePath pattern; removed builder example.
  • Breaking Change

    • Angular preset no longer re-exports from its distribution bundle; update imports to use the official entry points.

@greptile-apps greptile-apps Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

1 file reviewed, no comments

Edit Code Review Bot Settings | Greptile

@nx-cloud

nx-cloud Bot commented Sep 16, 2025

Copy link
Copy Markdown

View your CI Pipeline Execution ↗ for commit 321d812

Command Status Duration Result
nx run-many -t build --parallel=3 ✅ Succeeded 50s View ↗

☁️ Nx Cloud last updated this comment at 2025-09-25 07:39:34 UTC

@coderabbitai

coderabbitai Bot commented Sep 16, 2025

Copy link
Copy Markdown
Contributor

Walkthrough

This change set standardizes module path resolution across several packages and docs by:

  • Switching from path.join to template literals like ${pkg}/package.json when resolving package roots.
  • Using import.meta.resolve to locate package.json, combined with fileURLToPath and dirname where needed.
  • Updating core module resolution to use resolveModulePath from exsolve with a simplified try/catch.
  • Adjusting Angular preset paths to use fileURLToPath(import.meta.resolve(...)).
  • Updating test configs, automigration utilities, generators, and docs to the new resolution approach.
  • Removing an export-all re-export from code/frameworks/angular/preset.js.
  • No public API surface changes, except the removed re-export in Angular preset.

Sequence Diagram(s)

sequenceDiagram
  autonumber
  actor User as Caller (config/migration)
  participant GAP as getAbsolutePath
  participant IMR as import.meta.resolve
  participant URL as fileURLToPath
  participant NP as dirname

  User->>GAP: getAbsolutePath(value)
  Note right of GAP: Build specifier `${value}/package.json`
  GAP->>IMR: resolve(specifier)
  IMR-->>GAP: file: URL to package.json
  GAP->>URL: fileURLToPath(URL)
  URL-->>GAP: /abs/path/to/package.json
  GAP->>NP: dirname(/abs/path/to/package.json)
  NP-->>GAP: /abs/path
  GAP-->>User: /abs/path
Loading
sequenceDiagram
  autonumber
  actor SB as Call Site
  participant UM as utils/module.ts
  participant RMP as resolveModulePath (exsolve)
  participant FS as fs

  SB->>UM: resolveModulePathLike(parent, request, extensions)
  UM->>RMP: resolve({ from: parent, extensions: ["", ...extensions] })
  alt Resolved file path
    RMP-->>UM: /abs/path or file: URL
    UM->>FS: stat/isFile?
    FS-->>UM: is file
    UM-->>SB: resolved path
  else Error or not file
    RMP-->>UM: throws or non-file
    UM-->>SB: undefined
  end
Loading
sequenceDiagram
  autonumber
  actor TP as Telemetry
  participant PJR as getActualPackageJson
  participant IMR as import.meta.resolve

  TP->>PJR: getActualPackageJson(packageName)
  PJR->>IMR: resolve(`${packageName}/package.json`, cwd)
  IMR-->>PJR: URL or throws
  alt Resolved
    PJR-->>TP: package.json content/path (existing logic)
  else Fallback
    PJR-->>TP: previous fallback behavior (unchanged)
  end
Loading

Suggested reviewers

  • yannbf

Pre-merge checks and finishing touches

❌ Failed checks (1 warning)
Check name Status Explanation Resolution
Out of Scope Changes Check ⚠️ Warning Most changes are in-scope (safeResolveModule and getAbsolutePath fixes), but there are out-of-scope edits that warrant attention: the removal of the public re-export export * from './dist/preset.js' in code/frameworks/angular/preset.js is not referenced by the linked issues and can be a breaking API change, and the MIGRATION.md snippet removal of the builder entry alters documented configuration without explanation. These repository-wide or public-facing changes should be explicitly justified in the PR or reverted. Revert the Angular preset re-export removal unless it is intentional and then document/justify it in the PR and changelog and add compatibility tests; restore or explicitly explain the MIGRATION.md builder example removal or move that change to a separate PR that documents the rationale and impact.
✅ Passed checks (4 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title Check ✅ Passed The title succinctly describes the primary change: replacing safeResolveModule's resolution with exsolve.resolveModulePath to address Windows path issues. It is specific to the core area and the affected function, concise, and easily understood by reviewers.
Linked Issues Check ✅ Passed The changes implement the coding objectives from the linked issues: safeResolveModule now uses exsolve.resolveModulePath instead of import.meta.resolve/fileURLToPath, addressing the ERR_INVALID_URL_SCHEME Windows failure described in #32476, and the getAbsolutePath wrapper logic was changed from join(value, 'package.json') to ${value}/package.json across automigrations, generators, docs and tests, addressing #32485. These edits directly target the reported Windows/Yarn PnP path-resolution problems. No automated Windows-specific tests were added in this PR.
Docstring Coverage ✅ Passed No functions found in the changes. Docstring coverage check skipped.
✨ Finishing touches
  • 📝 Generate Docstrings
🧪 Generate unit tests
  • Create PR with unit tests
  • Post copyable unit tests in a comment

📜 Recent review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 61d7de7 and 321d812.

📒 Files selected for processing (1)
  • code/core/src/shared/utils/module.ts (2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
  • code/core/src/shared/utils/module.ts
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
  • GitHub Check: normal
  • GitHub Check: Core Unit Tests, windows-latest

Comment @coderabbitai help to get the list of available commands and usage tips.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)
code/core/src/telemetry/package-json.ts (1)

36-41: Fix path/URL mixing in fallback to avoid ERR_INVALID_URL_SCHEME (Windows).

import.meta.resolve() returns a file URL; passing that into pathToFileURL() (which expects a filesystem path) double-wraps and can throw on Windows. Convert the fallback result to a path first.

-      resolvedPackageJsonPath = import.meta.resolve(`${packageName}/package.json`, process.cwd());
+      resolvedPackageJsonPath = fileURLToPath(
+        import.meta.resolve(`${packageName}/package.json`, process.cwd())
+      );
🧹 Nitpick comments (4)
code/core/src/telemetry/package-json.ts (1)

32-35: Optional: normalize the parent argument to a URL.

Passing process.cwd() works in practice, but Node’s import.meta.resolve expects a URL-like parent. Using pathToFileURL(process.cwd()).href is more robust across runtimes.

-    let resolvedPackageJsonPath = pkg.up({
-      cwd: fileURLToPath(import.meta.resolve(packageName, process.cwd())),
-    });
+    let resolvedPackageJsonPath = pkg.up({
+      cwd: fileURLToPath(import.meta.resolve(packageName, pathToFileURL(process.cwd()).href)),
+    });
docs/_snippets/storybook-main-pnpm-with-module-resolution.md (1)

26-31: Remove unused import in TS snippet.

join is no longer used after switching to template literals.

-import { dirname, join } from 'node:path';
+import { dirname } from 'node:path';
code/lib/cli-storybook/src/automigrate/fixes/wrap-getAbsolutePath-utils.ts (1)

165-171: Provide cooked values in TemplateElement nodes.

@babel/types prefers both raw and cooked. Supplying only raw can trip tooling/print edge cases.

-                  t.templateLiteral(
-                    [
-                      t.templateElement({ raw: '' }),
-                      t.templateElement({ raw: '/package.json' }, true),
-                    ],
-                    [t.identifier('value')]
-                  ),
+                  t.templateLiteral(
+                    [
+                      t.templateElement({ raw: '', cooked: '' }),
+                      t.templateElement({ raw: '/package.json', cooked: '/package.json' }, true),
+                    ],
+                    [t.identifier('value')]
+                  ),
MIGRATION.md (1)

570-581: Clarify builder path vs file URL in example.

import.meta.resolve() typically returns a file URL; the text/comment shows a filesystem path. To avoid confusion (and to satisfy “fully resolved path”), either convert to a path or update the comment.

Option A (path string):

-import { dirname } from 'node:path';
+import { dirname } from 'node:path';
+import { fileURLToPath } from 'node:url';-  builder: import.meta.resolve('@storybook/builder-vite'),
-  // 👆 results in eg. `/absolute/path/node_modules/@storybook/builder-vite/index.js
+  builder: fileURLToPath(import.meta.resolve('@storybook/builder-vite')),
+  // 👆 results in e.g. `/absolute/path/node_modules/@storybook/builder-vite/index.js`

Option B (keep URL): adjust the comment to “file URL”.

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between c94db38 and 0fe2141.

📒 Files selected for processing (8)
  • MIGRATION.md (1 hunks)
  • code/core/src/telemetry/package-json.ts (1 hunks)
  • code/lib/cli-storybook/src/automigrate/fixes/__test__/main-config-with-wrappers.js (1 hunks)
  • code/lib/cli-storybook/src/automigrate/fixes/wrap-getAbsolutePath-utils.ts (2 hunks)
  • code/lib/cli-storybook/src/automigrate/fixes/wrap-getAbsolutePath.test.ts (2 hunks)
  • code/lib/cli-storybook/src/automigrate/fixes/wrap-getAbsolutePath.ts (1 hunks)
  • docs/_snippets/storybook-main-pnpm-with-module-resolution.md (2 hunks)
  • test-storybooks/yarn-pnp/.storybook/main.ts (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
  • test-storybooks/yarn-pnp/.storybook/main.ts
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
  • GitHub Check: Core Unit Tests, windows-latest
🔇 Additional comments (5)
code/lib/cli-storybook/src/automigrate/fixes/wrap-getAbsolutePath-utils.ts (2)

136-138: Docstring matches implementation.

Example now correctly shows the ${value}/package.json approach via import.meta.resolve. LGTM.


114-128: Confirm import injection for dirname/fileURLToPath.

This util generates a function using dirname and fileURLToPath. Ensure the automigration also inserts/imports these from node:path and node:url when missing.

Would you like a script that scans generated configs to verify the imports are present?

code/lib/cli-storybook/src/automigrate/fixes/__test__/main-config-with-wrappers.js (1)

1-6: LGTM: wrapper now resolves via URL → path → dir.

Matches the new guidance and is Windows-safe.

code/lib/cli-storybook/src/automigrate/fixes/wrap-getAbsolutePath.test.ts (2)

103-103: LGTM: snapshot now imports only dirname from node:path.

In sync with generator changes.


124-125: Confirm Node >=18.19.0 support and add a Vitest fallback for import.meta.resolve

Node unflagged import.meta.resolve starting in v18.19.0, but vite-node/Vitest often don’t implement import.meta.resolve in tests (errors like vite_ssr_import_meta.resolve is not a function). For the code at code/lib/cli-storybook/src/automigrate/fixes/wrap-getAbsolutePath.test.ts (lines 124–125) either ensure CI/local Node is >=18.19.0 or add a test/runtime fallback (stub vite_ssr_import_meta.resolve or use an import-meta-resolve ponyfill) so snapshots don’t break.

Comment on lines +75 to 77
mainConfig.setImport(['dirname'], 'node:path');
mainConfig.setImport(['fileURLToPath'], 'node:url');
}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

🧩 Analysis chain

LGTM: drop join import; keep only dirname.

Matches the new import.meta.resolve(\${value}/package.json`)` approach and avoids path-concat pitfalls.

Run to ensure no stale join(value, 'package.json') patterns remain in the repo:


🏁 Script executed:

#!/bin/bash
rg -nP -C2 '\bjoin\s*\(\s*value\s*,\s*["\']package\.json["\']\s*\)'

Length of output: 68


🏁 Script executed:

#!/bin/bash
set -euo pipefail
echo "Searching for exact join(value, 'package.json') patterns..."
rg -nP -C2 '\bjoin\s*\(\s*[^,)]*value\s*,\s*[\x27\x22]package\.json[\x27\x22]\s*\)' || echo "No exact matches"
echo
echo "Searching (looser) for lines that contain join(...value...package.json)..."
rg -nP -C2 'join\s*\(.*value.*package\.json' || echo "No looser matches"

Length of output: 2143


Fix remaining join(...) usage — replace with import.meta.resolve(${value}/package.json) and drop join import.

Occurrences: code/lib/create-storybook/src/generators/baseGenerator.ts — lines 410 and 417 (calls: return dirname(fileURLToPath(import.meta.resolve(join(value, 'package.json'))))).
Replace join(value, 'package.json') with ${value}/package.json inside import.meta.resolve and remove the join import.

🤖 Prompt for AI Agents
In code/lib/cli-storybook/src/automigrate/fixes/wrap-getAbsolutePath.ts around
lines 75-77 and in code/lib/create-storybook/src/generators/baseGenerator.ts at
lines ~410 and ~417, replace the use of join(value, 'package.json') inside
import.meta.resolve with a template literal `${value}/package.json`, and remove
any import of join from the module; specifically update the two calls that
currently do fileURLToPath(import.meta.resolve(join(value, 'package.json'))) to
use fileURLToPath(import.meta.resolve(`${value}/package.json`)) and delete the
join import and any dead code references.

On windows this will use the incorrect path separator, resulting in
unpredictable behaviour

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 0

🧹 Nitpick comments (4)
code/lib/create-storybook/src/generators/baseGenerator.ts (2)

399-419: Generated ESM imports: prefer node: specifiers and correct TS return type

  • Use node:-prefixed builtins in generated code to match Node ESM guidance and your docs.
  • The TS variant should return string, not any.

Apply this diff to the generated prefixes:

-          'import { dirname } from "path"',
-          'import { fileURLToPath } from "url"',
+          'import { dirname } from "node:path";',
+          'import { fileURLToPath } from "node:url";',
@@
-            function getAbsolutePath(value) {
-              return dirname(fileURLToPath(import.meta.resolve(`${value}/package.json`)))
+            function getAbsolutePath(value) {
+              return dirname(fileURLToPath(import.meta.resolve(`${value}/package.json`)));
             }`
             : dedent`/**
           * This function is used to resolve the absolute path of a package.
           * It is needed in projects that use Yarn PnP or are set up within a monorepo.
           */
-          function getAbsolutePath(value: string): any {
-            return dirname(fileURLToPath(import.meta.resolve(`${value}/package.json`)))
+          function getAbsolutePath(value: string): string {
+            return dirname(fileURLToPath(import.meta.resolve(`${value}/package.json`)));
           }`,

474-481: Resolve internal package root robustly

Minor: this path is resolved at runtime; if resolution ever fails in monorepos, adding a base to import.meta.resolve avoids edge cases.

-      commonAssetsDir: join(
-        dirname(fileURLToPath(import.meta.resolve('create-storybook/package.json'))),
+      commonAssetsDir: join(
+        dirname(fileURLToPath(import.meta.resolve('create-storybook/package.json', import.meta.url))),
         'rendererAssets',
         'common'
       ),
MIGRATION.md (2)

570-574: Doc snippet uses require.resolve in ESM: add createRequire for correctness

Readers copying this into ESM will hit “require is not defined.” Show createRequire.

-import { dirname } from 'node:path';
-
-const getAbsolutePath = (input) =>
-  dirname(require.resolve(`${input}/package.json`));
+import { dirname } from 'node:path';
+import { createRequire } from 'node:module';
+const require = createRequire(import.meta.url);
+const getAbsolutePath = (input) =>
+  dirname(require.resolve(`${input}/package.json`));

579-581: Builder example: clarify URL vs path

import.meta.resolve('@storybook/builder-vite') returns a URL string. Consider adding a short note or fileURLToPath in the example to prevent confusion for consumers expecting a filesystem path.

Suggested edit:

-  builder: import.meta.resolve('@storybook/builder-vite'),
+  // Resolve to a file URL, convert to a path if needed:
+  // builder: fileURLToPath(import.meta.resolve('@storybook/builder-vite')),
+  builder: import.meta.resolve('@storybook/builder-vite'),
📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 0fe2141 and 696eb54.

📒 Files selected for processing (9)
  • MIGRATION.md (1 hunks)
  • code/core/src/telemetry/package-json.ts (1 hunks)
  • code/lib/cli-storybook/src/automigrate/fixes/__test__/main-config-with-wrappers.js (1 hunks)
  • code/lib/cli-storybook/src/automigrate/fixes/wrap-getAbsolutePath-utils.ts (2 hunks)
  • code/lib/cli-storybook/src/automigrate/fixes/wrap-getAbsolutePath.test.ts (2 hunks)
  • code/lib/cli-storybook/src/automigrate/fixes/wrap-getAbsolutePath.ts (1 hunks)
  • code/lib/create-storybook/src/generators/baseGenerator.ts (1 hunks)
  • docs/_snippets/storybook-main-pnpm-with-module-resolution.md (2 hunks)
  • test-storybooks/yarn-pnp/.storybook/main.ts (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (6)
  • test-storybooks/yarn-pnp/.storybook/main.ts
  • code/lib/cli-storybook/src/automigrate/fixes/wrap-getAbsolutePath.test.ts
  • code/lib/cli-storybook/src/automigrate/fixes/wrap-getAbsolutePath.ts
  • code/lib/cli-storybook/src/automigrate/fixes/wrap-getAbsolutePath-utils.ts
  • code/core/src/telemetry/package-json.ts
  • docs/_snippets/storybook-main-pnpm-with-module-resolution.md
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
  • GitHub Check: Core Unit Tests, windows-latest
🔇 Additional comments (1)
code/lib/cli-storybook/src/automigrate/fixes/__test__/main-config-with-wrappers.js (1)

1-1: ESM builtins: keep specifiers consistent and verify Windows behavior

  • node: specifiers are fine. Found wrapForPnp usage in code/lib/cli-storybook/src/automigrate/fixes/test/main-config-with-wrappers.js (addons and framework.name). import.meta.resolve can produce file:// URLs with forward slashes on Windows (Node 20.19+); verify on Windows + Yarn PnP that the paths passed to wrapForPnp are normalized/converted (e.g., fileURLToPath or path.normalize) to avoid path-format regressions.

@valentinpalkovic valentinpalkovic added maintenance User-facing maintenance tasks ci:normal Run our default set of CI jobs (choose this for most PRs). cli core and removed cli labels Sep 22, 2025
@storybook-app-bot

storybook-app-bot Bot commented Sep 22, 2025

Copy link
Copy Markdown

Package Benchmarks

Commit: 321d812, ran on 29 September 2025 at 11:05:07 UTC

The following packages have significant changes to their size or dependencies:

@storybook/nextjs-vite

Before After Difference
Dependency count 124 124 0
Self size 4.10 MB 4.01 MB 🎉 -81 KB 🎉
Dependency size 21.62 MB 21.62 MB 🚨 +18 B 🚨
Bundle Size Analyzer Link Link

@storybook/sveltekit

Before After Difference
Dependency count 20 20 0
Self size 58 KB 48 KB 🎉 -11 KB 🎉
Dependency size 26.85 MB 26.85 MB 🎉 -498 B 🎉
Bundle Size Analyzer Link Link

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 0

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)
code/frameworks/angular/src/preset.ts (1)

15-28: Avoid mixing file URLs and file paths in annotations

config is a file path, but previewProdPath and docsConfigPath remain file URLs. Normalize all entries to file paths to prevent cross‑platform issues (esp. Windows) and to match other frameworks (see nextjs/sveltekit presets).

Apply this diff:

   const config = fileURLToPath(import.meta.resolve('@storybook/angular/client/config'));
   const annotations = [...entries, config];

   if ((options as any as StandaloneOptions).enableProdMode) {
-    const previewProdPath = import.meta.resolve('@storybook/angular/client/preview-prod');
-    annotations.unshift(previewProdPath);
+    const previewProdPath = fileURLToPath(
+      import.meta.resolve('@storybook/angular/client/preview-prod')
+    );
+    annotations.unshift(previewProdPath);
   }

   const docsConfig = await options.presets.apply('docs', {}, options);
   const docsEnabled = Object.keys(docsConfig).length > 0;
   if (docsEnabled) {
-    const docsConfigPath = import.meta.resolve('@storybook/angular/client/docs/config');
-    annotations.push(docsConfigPath);
+    const docsConfigPath = fileURLToPath(
+      import.meta.resolve('@storybook/angular/client/docs/config')
+    );
+    annotations.push(docsConfigPath);
   }
🧹 Nitpick comments (1)
code/frameworks/angular/src/preset.ts (1)

36-40: Consider normalizing builder name resolution

import.meta.resolve returns a file URL. Prefer passing either the package specifier or a file path for consistency with the Windows fix. Verify which form the core expects here.

Two safe options (pick one, confirm e2e on Windows/Yarn PnP):

-      name: import.meta.resolve('@storybook/builder-webpack5'),
+      name: '@storybook/builder-webpack5',

or

-      name: import.meta.resolve('@storybook/builder-webpack5'),
+      name: fileURLToPath(import.meta.resolve('@storybook/builder-webpack5')),
📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 696eb54 and 10c1a03.

📒 Files selected for processing (1)
  • code/frameworks/angular/src/preset.ts (1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
code/frameworks/angular/src/preset.ts (4)
code/frameworks/react-webpack5/src/preset.ts (1)
  • addons (9-11)
code/frameworks/nextjs/src/preset.ts (2)
  • addons (19-21)
  • previewAnnotations (50-53)
code/frameworks/nextjs-vite/src/preset.ts (1)
  • previewAnnotations (37-40)
code/frameworks/sveltekit/src/preset.ts (1)
  • previewAnnotations (16-19)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
  • GitHub Check: normal
  • GitHub Check: Core Unit Tests, windows-latest
🔇 Additional comments (2)
code/frameworks/angular/src/preset.ts (2)

4-4: LGTM: importing fileURLToPath is correct for resolving to absolute paths

This aligns with the Windows fix strategy.


7-8: LGTM: resolve addon presets to file paths

Good move to absolute file paths via fileURLToPath(import.meta.resolve(...)).

@ndelangen ndelangen changed the title Use exsolve's resolveModulePath for safeResolveModule Core: Use exsolve resolveModulePath for safeResolveModule Sep 24, 2025

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 0

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)
code/core/src/common/presets.ts (1)

81-84: Use POSIX-style module specifiers — avoid pathe.join for specifiers

join(name, 'preset'|'manager'|'preview') can produce platform-specific separators (\ on Windows); replace with POSIX-style ${name}/... and remove the now-unused join import.

File: code/core/src/common/presets.ts

-  const presetFile = safeResolveModule({ specifier: join(name, 'preset'), parent: configDir });
-  const managerFile = safeResolveModule({ specifier: join(name, 'manager'), parent: configDir });
-  const previewFile = safeResolveModule({ specifier: join(name, 'preview'), parent: configDir });
+  const presetFile = safeResolveModule({ specifier: `${name}/preset`, parent: configDir });
+  const managerFile = safeResolveModule({ specifier: `${name}/manager`, parent: configDir });
+  const previewFile = safeResolveModule({ specifier: `${name}/preview`, parent: configDir });

Also remove the unused join import:

-import { join, parse, resolve } from 'pathe';
+import { parse, resolve } from 'pathe';
🧹 Nitpick comments (1)
code/core/src/common/presets.ts (1)

239-242: Align error logging with stack/message for consistency

Mirror the improved logging used above so stack traces are printed reliably here as well.

Apply this diff:

-    logger.warn(warning);
-    logger.error(error);
+    logger.warn(warning);
+    const err = error as Error;
+    logger.error(err.stack ?? err.message);
📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 10c1a03 and 83663e2.

📒 Files selected for processing (1)
  • code/core/src/common/presets.ts (1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
  • GitHub Check: normal
  • GitHub Check: Core Unit Tests, windows-latest
🔇 Additional comments (1)
code/core/src/common/presets.ts (1)

131-133: Useful stack-aware error logging added — LGTM

Logging error.stack ?? error.message will make failures much easier to diagnose.

Comment thread code/core/src/shared/utils/module.ts Outdated
Comment on lines +125 to +127
if (e.code !== 'ERR_MODULE_NOT_FOUND') {
throw e;
}

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is the cause of the error @mrginglymus

Can you explain why you added this?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've removed the code, for now.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Because the old logic would iterate through extensions it was expected that at least some of them would throw, but that's ok because the throw just said 'move on to the next extension'- they are recoverable exceptions.

However, this was also catching and thus masking the critical error from fileURLToPath (or whichever function was choking on windows absolute paths).

I'd hoped that being a bit more selective in error codes would allow to differentiate between 'the module isn't there, let's just carry on' (ie, the existing behaviour) and 'there's a critical configuration error or bug in storybook that needs fixing'.

Obviously I've missed the target there and we're getting more than just ERR_MODULE_NOT_FOUND on the happy path.

@jonniebigodes jonniebigodes left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@mrginglymus thanks for taking the time to update the documentation. Appreciate it 🙏 ! I've checked and all is good on my end. I'll defer to @ndelangen on this, as he left some items for you to check when you're able.

Have a great day.

Stay safe

@ndelangen ndelangen merged commit 1a31c48 into storybookjs:next Sep 25, 2025
53 of 54 checks passed
@github-actions github-actions Bot mentioned this pull request Sep 25, 2025
16 tasks
@coderabbitai coderabbitai Bot mentioned this pull request Oct 6, 2025
8 tasks
@yannbf yannbf removed the needs qa label Nov 24, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

ci:normal Run our default set of CI jobs (choose this for most PRs). core maintenance User-facing maintenance tasks

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Bug]: getAbsolutePath wrapper incorrect on windows [Bug]: 10.0.0-beta.3 continued windows issues

5 participants